Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3단계 - 즐겨찾기 기능 구현 #355

Open
wants to merge 29 commits into
base: hscom96
Choose a base branch
from

Conversation

hscom96
Copy link

@hscom96 hscom96 commented Oct 1, 2022

리뷰어님 안녕하세요!

드디어 마지막 단계 제출합니다 ㅎㅎ
지난 단계 꼼꼼히 리뷰해주셔서 감사합니다.

시간 나실때 천천히 리뷰 해주셔도 괜찮습니다. 😄

Copy link
Member

@testrace testrace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 현수님! 😃
지난 피드백 반영과 즐겨찾기 기능 구현 잘해주셨네요 👍
예외 케이스에 대한 검증을 조금만 더 보완했으면 합니다!
관련 내용을 코멘트로 남겼으니 확인 부탁드리겠습니다

src/main/java/nextstep/auth/AuthConfig.java Show resolved Hide resolved
UserDetail loadUserByUsername(String email);
User loadUserByUsername(String email);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UserDetail 인터페이스를 제거하고 User 클래스로 대체하기 보다
User 클래스가 UserDetail 인터페이스를 구현하는 형태는 어떨까요?

Copy link
Author

@hscom96 hscom96 Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 먼저 말씀하신대로 다음 PR과 같이 구현 해봤는데 0e880a2

말씀하신 부분에서 궁금한점이 있습니다.

인터페이스 (ex. UserDetail) 사용의 효과중 하나로 의존성 역전을 위한것으로 이해했는데
제가 만든 프로젝트에서는 member 패키지에서 LoginMember가 삭제되고, User객체는 auth 패키지에만 있어서
기존처럼 User클래스가 UserDetail 인터페이스를 상속 안해줘도 member에서 auth패키지로 의존성이 한방향으로만 흐르고 있고

또한 User와 UserDetail이 같은 패키지 내부에 있는데

여기서 User가 UserDetail을 구현하는 장점이 어떤게 있을지 알 수 있을까요? 🤔

public class FavoritesController {
private final FavoritesService favoritesService;

@Secured(RoleType.ROLE_ADMIN)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요구사항

  • 예외 케이스에 대한 검증도 포함하세요.
    • 로그인이 필요한 API 요청 시 유효하지 않은 경우 401 응답 내려주기

관리자가 아닌 일반 회원, 비로그인 사용자에 따라 예외를 처리할 수 있게 변경해 보시면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기존 테스트에 일반 회원, 비로그인 사용자에 대한 예외 테스트를 보강했습니다 !!
e194e54

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants